Skip to content

Fix #1083: free CJamRecorder if Init() fails - #3838

Merged
ann0see merged 2 commits into
jamulussoftware:mainfrom
mcfnord:fix-1083-recorder-leak
Jul 30, 2026
Merged

Fix #1083: free CJamRecorder if Init() fails#3838
ann0see merged 2 commits into
jamulussoftware:mainfrom
mcfnord:fix-1083-recorder-leak

Conversation

@mcfnord

@mcfnord mcfnord commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Note

📡 STAND BY FOR AN LLM-AUTHORED MESSAGE.

Fixes the one definitely lost shape from the original valgrind report (the "possibly lost" shapes are the well-known glibc/Qt TLS false-positive pattern and aren't touched here).

The leak: CJamController::SetRecordingDir() (src/recorder/jamcontroller.cpp) unconditionally does pJamRecorder = new CJamRecorder(...), then calls Init(). The moveToThread + deleteLater cleanup wiring only happens inside the if (bRecorderInitialised) branch below it. If Init() fails — e.g. an unwritable recording directory — the freshly-new'd object has no parent, no thread affinity, and nothing connected to deleteLater. CJamController has no destructor, and CJamRecorder's own constructor takes no parent, so nothing else ever frees it. The next call to SetRecordingDir() overwrites pJamRecorder, so the leak repeats on every failed directory change.

The fix: delete it immediately in the failure branch, right where bRecorderInitialised is set. It's provably safe to delete unconditionally here — this object is only reachable via the local pJamRecorder pointer at this point, never moved to a thread, never connected to anything. (The other branch, further down — a running recorder switching to an empty dir — is a different object already scheduled via deleteLater; deleting there would double-free, so the fix is scoped only to this one failure path.)

        if ( !bRecorderInitialised )
        {
            delete pJamRecorder;   // Init() failed: never moved to thread / never
            pJamRecorder = nullptr;//   wired to deleteLater, so free it here (#1083)
        }

Verification: built headless/serveronly locally (qmake CONFIG+=headless CONFIG+=serveronly && make), clean compile, binary runs. I don't have a way to re-run the original valgrind report here, but the fix is small enough to review by inspection — happy to add a regression test if maintainers want one (e.g. drive SetRecordingDir with an unwritable path and assert no CJamRecorder outlives the call).

Fixes #1083.

@ann0see
ann0see requested a review from pljones July 27, 2026 10:45
Comment thread src/recorder/jamcontroller.cpp
@pljones

pljones commented Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Looks good apart from the coding style check fail.

@dingodoppelt dingodoppelt added the AI AI generated or potentially AI generated label Jul 28, 2026
@ann0see
ann0see requested a review from softins July 28, 2026 20:21
@ann0see ann0see added this to the Release 3.12.4 milestone Jul 28, 2026
@ann0see ann0see added this to Tracking Jul 28, 2026
@github-project-automation github-project-automation Bot moved this to Triage in Tracking Jul 28, 2026
@ann0see ann0see moved this from Triage to Waiting on Team in Tracking Jul 28, 2026
@ann0see

ann0see commented Jul 28, 2026

Copy link
Copy Markdown
Member

Please squash the clang format fix.

SetRecordingDir() unconditionally news a CJamRecorder, then only wires
moveToThread/deleteLater cleanup inside the bRecorderInitialised branch.
If Init() fails (e.g. unwritable directory), the freshly-new'd object is
orphaned: no thread affinity, no deleteLater connection, no destructor
anywhere else to catch it. Free it immediately in the failure branch.

The next SetRecordingDir() call overwrites pJamRecorder, so without this
fix the leak repeats on every failed recording-dir change.
@mcfnord
mcfnord force-pushed the fix-1083-recorder-leak branch from 203bb60 to 2cd4ad0 Compare July 29, 2026 02:59
Comment thread src/recorder/jamcontroller.cpp Outdated
if ( !bRecorderInitialised )
{
delete pJamRecorder; // Init() failed: never moved to thread / never
pJamRecorder = nullptr; // wired to deleteLater, so free it here (#1083)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly dodgy formatting if you want to fix that. LLMs cannot count - particularly spaces.

Comment thread src/recorder/jamcontroller.cpp Outdated

@softins softins left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ann0see
ann0see merged commit 9b7e08f into jamulussoftware:main Jul 30, 2026
15 checks passed
@github-project-automation github-project-automation Bot moved this from Waiting on Team to Done in Tracking Jul 30, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AI AI generated or potentially AI generated

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

Memory leak: Investigate valgrind output after #1073

5 participants